Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove google fonts by serving them locally #1188

Merged
merged 3 commits into from May 20, 2020
Merged

Remove google fonts by serving them locally #1188

merged 3 commits into from May 20, 2020

Conversation

pheki
Copy link
Contributor

@pheki pheki commented Apr 14, 2020

This is #848 +

  • Apache2 license added to the fonts directory with link in the .css file (edit: I changed the link to the license in the apache site)
  • All scripts (charsets) added to the fonts and they're also updated (used https://google-webfonts-helper.herokuapp.com/fonts/)
  • Other minor improvements

Fixes #847

I may also remove the .woff variants as the only current browser that doesn't support it is IE, which would anyway fallback to sans-serif.

I propose that we first fix this long standing issue and then add an option to make these fonts optional for those that use custom themes, for example.

@ehuss can you give me your thoughts?

Thanks!

@ehuss
Copy link
Contributor

ehuss commented May 10, 2020

Thanks for picking this up!

Just a few small things, and I think I'd be happy merging this:

  • I think Source Code Pro needs a copy of the OFL license (at least the terms seem to indicate it needs to be included when bundled).
  • I'd be happy to drop the woff fonts. AFAIK, the older browsers which just use default fonts instead, right?
  • I think there should be a mechanism to skip copying the fonts to the output directory if you are not using them. I think I personally would prefer, if fonts.css is overridden, to skip copying the fonts. I think that would be nice, as it would avoid extra configuration needed, and if you are overriding fonts.css, you're probably specifying your own fonts anyways. I'm not sure how difficult that will be to detect, though.
  • The documentation needs to be updated. I think theme/README.md would be a good place to indicate that fonts can be overridden, and what the implications are.

Co-authored-by: Aral Balkan <aral@ind.ie>
Co-authored-by: Collyn O'Kane <47607823+okaneco@users.noreply.github.com>
@pheki
Copy link
Contributor Author

pheki commented May 15, 2020

Hey!

First of all, I didn't want the .woff files to end up in the git history (as they sum more than 500 kB) and the commits were very confusing, so I just force pushed a single simpler commit with the authors of both #848 and #1189 as co-authors.

Addressing your points:

  • OFL license added 👍
  • Yes, older browsers will fallback to system fonts, up to the last font specified in general.css). In the case of Open Sans any sans-serif font will be used and in the case of Source Code Pro any monospace font will be used (with a lot of fallbacks apparently)
  • I've actually implemented skipping copying fonts when fonts.css is overridden, but found it pretty clunky. For example, creating a theme with mdbook init --theme (which generates the default theme) would stop fonts from being copied. I've kept that implementation at another branch. The implementation in this PR actually adds fonts.css to /fonts and not to the theme, preventing it and all fonts from being copied with a new option: no-copy-fonts. I'd like it better for the option to be copy-fonts but then I'd have we'd have to provide impl Default for HtmlConfig instead of deriving and I'm not sure about your opinion on this.
  • Docs for the configuration file updated.

What do you think?

@tesuji
Copy link
Contributor

tesuji commented May 15, 2020

I didn't want the .woff files to end up in the git history

How about using build.rs to download fonts at build time?

@ehuss
Copy link
Contributor

ehuss commented May 15, 2020

Ah, good catch! I think I would prefer to avoid the no- prefix. I think it would be fine to have a manual Default impl.

@ehuss
Copy link
Contributor

ehuss commented May 15, 2020

How about using build.rs to download fonts at build time?

I would prefer not to have a network dependency like that.

@pheki
Copy link
Contributor Author

pheki commented May 15, 2020

Ah, good catch! I think I would prefer to avoid the no- prefix. I think it would be fine to have a manual Default impl.

Great! In the next few days I'll fix this.

@pheki
Copy link
Contributor Author

pheki commented May 19, 2020

Done! I also forgot to copy the fonts' licenses themselves to the output directory, which is now fixed 😅 .

Copy link
Contributor

@ehuss ehuss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again for the help!

@ehuss ehuss merged commit e677b72 into rust-lang:master May 20, 2020
@pheki pheki deleted the remove-google-surveillance branch May 22, 2020 03:07
Ruin0x11 pushed a commit to Ruin0x11/mdBook that referenced this pull request Aug 30, 2020
Remove google fonts by serving them locally
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove Google surveillance
3 participants